Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(rethinkdb): OrganizationUser: Phase 2 #9953

Merged
merged 69 commits into from
Jul 11, 2024

Conversation

mattkrick
Copy link
Member

@mattkrick mattkrick commented Jul 10, 2024

Description

Migrates old data over to PG.
About 180 rows didn't make it over because the underlying user did not exist in the DB

Summary by CodeRabbit

  • New Features

    • Simplified user interface for billing and organization management by removing unnecessary fields.
    • Enhanced organization and user management logic for better performance and maintainability.
  • Bug Fixes

    • Improved consistency and reliability in user role updates and organization user counts.
  • Refactor

    • Unified database operations, transitioning from RethinkDB to PostgreSQL for streamlined backend processes.
  • Tests

    • Enhanced test coverage for organization user roles and tier management scenarios.

Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
…n-phase3

Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Copy link
Contributor

coderabbitai bot commented Jul 10, 2024

Walkthrough

The recent changes across various files primarily focus on improving the handling of user counts, subscription management, and billing within the organization's context. Key modifications include the removal of fields and properties related to newUserUntil, the use of the Kysely query builder for database operations, and the refactoring of logic to enhance performance and readability. These updates streamline data handling, ensure consistency, and optimise the overall billing and subscription processes.

Changes

File Path Change Summary
packages/client/components/BillingLeaderActionMenu.tsx Simplified component functionality by removing billingTier and newUserUntil fields and related logic.
packages/client/modules/userDashboard/components/OrgUserRow/OrgMemberRow.tsx Removed newUserUntil property from UserInfoProps interface and related component logic.
packages/server/billing/helpers/adjustUserCount.ts Refactored various functions to use Kysely for database operations, added dataLoader parameter to maybeUpdateOrganizationActiveDomain, and made other enhancements.
packages/server/billing/helpers/generateInvoice.ts Replaced direct database queries with dataLoader for fetching organizationUsers.
packages/server/billing/helpers/handleEnterpriseOrgQuantityChanges.ts Improved handling of organization user counts and subscription items, removed direct database calls, and optimised logic.
packages/server/billing/helpers/teamLimitsCheck.ts Added updates to the OrganizationUser table based on conditions in enableUsageStats and maybeRemoveRestrictions functions.
packages/server/billing/helpers/updateSubscriptionQuantity.ts Modified to use Kysely for database queries and added logic for handling mismatches in orgUserCount.
packages/server/database/types/OrganizationUser.ts Removed newUserUntil property from the Input interface and the class constructor, with the tier property defaulting to 'starter'.
packages/server/dataloader/__tests__/isOrgVerified.test.ts Added tier property and refactored insertion logic for Organization and OrganizationUser in tests.
packages/server/graphql/mutations/archiveOrganization.ts Introduced updates to the OrganizationUser table using Kysely, enhancing the archiving process.
packages/server/graphql/mutations/helpers/createNewOrg.ts Refactored query construction logic for inserting into Organization and OrganizationUser tables using nested queries.
packages/server/graphql/mutations/helpers/oldUpgradeToTeamTier.ts Refactored to use dataLoader for fetching organization users and filtering active users, removing direct RethinkDB queries.
packages/server/graphql/mutations/helpers/removeFromOrg.ts Updated logic for updating OrganizationUser records and handling billing leaders using Kysely.
packages/server/graphql/mutations/moveTeamToOrg.ts Refactored retrieval and handling logic for organizationUsers, including optional chaining for role checks and restructuring updates for efficiency.
packages/server/graphql/mutations/oldUpgradeToTeamTier.ts Added database update operation using Kysely to set the role of an OrganizationUser based on conditions.
packages/server/graphql/private/mutations/autopauseUsers.ts Added Kysely for database operations and included a query to fetch data based on user inactivity.
packages/server/graphql/private/mutations/backupOrganization.ts Removed the organizationUser mutation resolver, impacting data insertion logic during organization backup.
packages/server/graphql/private/mutations/checkRethinkPgEquality.ts Updated equality checks to handle OrganizationUser table comparisons, focusing on different fields and equality functions.
packages/server/graphql/private/mutations/connectSocket.ts Refactored to remove RethinkDB dependency, added handleInactive function for user inactivity logic using DataLoaderWorker.
packages/server/graphql/private/mutations/draftEnterpriseInvoice.ts Added database update operation for setting the role of an organization user to BILLING_LEADER using Kysely.
packages/server/graphql/private/mutations/stripeFailPayment.ts Simplified logic for fetching billingLeaderUserIds by using orgUsers instead of direct database queries.
packages/server/graphql/private/mutations/toggleAllowInsights.ts Added PostgreSQL query execution to update the OrganizationUser table based on specific conditions for toggling insights.
packages/server/graphql/public/mutations/acceptRequestToJoinDomain.ts Updated to use dataLoader for fetching organizationUsers instead of direct database queries.
packages/server/graphql/public/mutations/createStripeSubscription.ts Modified data loading process for organizationUsers to use dataLoader and directly filter and load organizationUsersByOrgId.
scripts/toolboxSrc/setIsEnterprise.ts Refactored to use a single database connection for PostgreSQL operations, updating tier information for various tables.
packages/server/graphql/public/subscriptions/organizationSubscription.ts Updated to include SubscriptionContext type in the SubscriptionResolvers declaration and use dataLoader for fetching organizationUsers.
packages/server/utils/__tests__/isRequestToJoinDomainAllowed.test.ts Added tier field to OrganizationUser type, refactored database insertion logic using Kysely, and extended test cases to cover various scenarios.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 32

Outside diff range, codebase verification and nitpick comments (11)
packages/server/utils/getActiveDomainForOrgId.ts (1)

1-3: Organise imports.

Consider grouping the imports logically (external libraries, internal modules, etc.) to improve readability.

+import getKysely from '../postgres/getKysely'
import {DataLoaderInstance} from '../dataloader/RootDataLoader'
packages/server/graphql/queries/helpers/countTiersForUserId.ts (2)

1-3: Organise imports.

Consider grouping the imports logically (external libraries, internal modules, etc.) to improve readability.

// breaking this out into its own helper so it can be used directly to
// populate segment traits
+import {DataLoaderInstance} from '../../../dataloader/RootDataLoader'

Line range hint 6-18:
Ensure proper error handling.

The function lacks error handling for data loading operations. Consider adding try-catch blocks to handle potential errors gracefully.

+try {
  const allOrgUsers = await dataLoader.get('organizationUsersByUserId').load(userId)
  const organizationUsers = allOrgUsers.filter(({inactive}) => !inactive)
  const tierStarterCount = organizationUsers.filter(
    (organizationUser) => organizationUser.tier === 'starter'
  ).length
  const tierTeamCount = organizationUsers.filter(
    (organizationUser) => organizationUser.tier === 'team'
  ).length
  const tierEnterpriseCount = organizationUsers.filter(
    (organizationUser) => organizationUser.tier === 'enterprise'
  ).length
  const tierTeamBillingLeaderCount = organizationUsers.filter(
    (organizationUser) =>
      organizationUser.tier === 'team' && organizationUser.role === 'BILLING_LEADER'
  ).length
  return {
    tierStarterCount,
    tierTeamCount,
    tierEnterpriseCount,
    tierTeamBillingLeaderCount
  }
+} catch (error) {
+  console.error('Failed to count tiers for userId:', userId, error)
+  throw error
+}
packages/server/utils/setTierForOrgUsers.ts (1)

Line range hint 1-3:
Remove unused import.

The getRethink import is no longer needed and should be removed to clean up the code.

-import getRethink from '../database/rethinkDriver'
import getKysely from '../postgres/getKysely'
packages/server/graphql/private/queries/suOrgCount.ts (1)

18-19: Remove debug code before production deployment.

The console.log(pgResults) statement should be removed or replaced with a proper logging mechanism before production deployment.

-  console.log(pgResults)
+  // console.log(pgResults) // Uncomment for debugging
packages/server/graphql/private/queries/suProOrgInfo.ts (1)

35-35: Remove debug code before production deployment.

The console.log({pgResults, activeOrgIds}) statement should be removed or replaced with a proper logging mechanism before production deployment.

-  console.log({pgResults, activeOrgIds})
+  // console.log({pgResults, activeOrgIds}) // Uncomment for debugging
packages/server/billing/helpers/handleEnterpriseOrgQuantityChanges.ts (1)

13-14: Use consistent indentation for array elements.

Ensure that the elements of the array passed to Promise.all are consistently indented for better readability.

-  const [orgUsers, subscriptionItem] = await Promise.all([
-    dataLoader.get('organizationUsersByOrgId').load(orgId),
-    manager.getSubscriptionItem(stripeSubscriptionId)
+  const [orgUsers, subscriptionItem] = await Promise.all([
+      dataLoader.get('organizationUsersByOrgId').load(orgId),
+      manager.getSubscriptionItem(stripeSubscriptionId)
  ])
packages/server/postgres/migrations/1720556055134_OrganizationUser-phase1.ts (2)

36-38: Consider index naming conventions.

Consider following a consistent naming convention for indexes to improve readability and maintainability.

-    CREATE INDEX IF NOT EXISTS "idx_OrganizationUser_tier" ON "OrganizationUser"("tier") WHERE "removedAt" IS NULL AND "inactive" = FALSE;
-    CREATE INDEX IF NOT EXISTS "idx_OrganizationUser_orgId" ON "OrganizationUser"("orgId") WHERE "removedAt" IS NULL;
-    CREATE INDEX IF NOT EXISTS "idx_OrganizationUser_userId" ON "OrganizationUser"("userId") WHERE "removedAt" IS NULL;
+    CREATE INDEX IF NOT EXISTS "organization_user_tier_idx" ON "OrganizationUser"("tier") WHERE "removedAt" IS NULL AND "inactive" = FALSE;
+    CREATE INDEX IF NOT EXISTS "organization_user_org_id_idx" ON "OrganizationUser"("orgId") WHERE "removedAt" IS NULL;
+    CREATE INDEX IF NOT EXISTS "organization_user_user_id_idx" ON "OrganizationUser"("userId") WHERE "removedAt" IS NULL;

49-51: Ensure proper cleanup in down function.

The down function in 1720556055134_OrganizationUser-phase1.ts does not handle dropping indexes or constraints associated with the OrganizationUser table. Please ensure that all associated indexes and constraints are dropped to complete the cleanup process.

  • Add DROP INDEX and DROP CONSTRAINT statements for any indexes and constraints related to the OrganizationUser table.
Analysis chain

Ensure proper cleanup in down function.

Ensure that all associated indexes and constraints are dropped in the down function.


Let's search the migration files for any DROP INDEX, DROP CONSTRAINT, or similar statements to ensure proper cleanup in the down function.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all associated indexes and constraints are dropped in the `down` function.

# Test: Check the table schema after running the `down` function to ensure all indexes and constraints are removed.
psql -c '\d+ "OrganizationUser"'

Length of output: 106


Script:

#!/bin/bash
# Search for DROP INDEX, DROP CONSTRAINT, or similar statements in the migration files to ensure proper cleanup.
rg 'DROP INDEX|DROP CONSTRAINT|DROP SEQUENCE|DROP TRIGGER' --glob '**/migrations/*.ts'

Length of output: 8815

packages/server/postgres/migrations/1720640784862_OrganizationUser-phase2.ts (1)

43-44: Initialise cursor values

Ensure that the cursor values curjoinedAt and curId are initialized to appropriate minimum values.

packages/server/dataloader/__tests__/isOrgVerified.test.ts (1)

80-81: Remove redundant null checks.

The removedAt field is already being set to null if not provided, making the null checks redundant. Consider removing them for cleaner code.

-    inactive: member.inactive ?? false,
-    role: member.role ?? null,
-    removedAt: member.removedAt ?? null,
+    inactive: member.inactive ?? false,
+    role: member.role ?? null,
+    removedAt: member.removedAt,

Comment on lines 78 to 81
const orgUsers = await dataLoader.get('organizationUsersByOrgId').load(orgId)
const billingLeaderOrgUsers = orgUsers.filter(
({role}) => role && ['BILLING_LEADER', 'ORG_ADMIN'].includes(role)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure the role field is always present.

The filter condition assumes that the role field is always present in the organization user object. Consider adding a null check or default value to avoid potential runtime errors.

-  const billingLeaderOrgUsers = orgUsers.filter(
-    ({role}) => role && ['BILLING_LEADER', 'ORG_ADMIN'].includes(role)
-  )
+  const billingLeaderOrgUsers = orgUsers.filter(
+    (user) => user.role && ['BILLING_LEADER', 'ORG_ADMIN'].includes(user.role)
+  )
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const orgUsers = await dataLoader.get('organizationUsersByOrgId').load(orgId)
const billingLeaderOrgUsers = orgUsers.filter(
({role}) => role && ['BILLING_LEADER', 'ORG_ADMIN'].includes(role)
)
const orgUsers = await dataLoader.get('organizationUsersByOrgId').load(orgId)
const billingLeaderOrgUsers = orgUsers.filter(
(user) => user.role && ['BILLING_LEADER', 'ORG_ADMIN'].includes(user.role)
)

.nth(0)
.default(null)
.run(),
dataLoader.get('organizationUsersByUserIdOrgId').load({orgId, userId}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for load method.

The dataLoader.get('organizationUsersByUserIdOrgId').load method might throw an error if the data loading fails. Consider adding error handling to manage this scenario gracefully.

-      dataLoader.get('organizationUsersByUserIdOrgId').load({orgId, userId}),
+      dataLoader.get('organizationUsersByUserIdOrgId').load({orgId, userId}).catch((error) => {
+        Logger.log(error)
+        return null
+      }),
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
dataLoader.get('organizationUsersByUserIdOrgId').load({orgId, userId}),
dataLoader.get('organizationUsersByUserIdOrgId').load({orgId, userId}).catch((error) => {
Logger.log(error)
return null
}),

@@ -40,7 +40,7 @@ export default {
// RESOLUTION
const {stripeId} = await dataLoader.get('organizations').loadNonNull(orgId)
const dbAfter = after ? new Date(after) : r.maxval
const [tooManyInvoices, orgUserCount] = await Promise.all([
const [tooManyInvoices, orgUsers] = await Promise.all([
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace RethinkDB maxval with Date object

Using r.maxval is specific to RethinkDB and should be replaced with an appropriate PostgreSQL compatible value, such as new Date(8640000000000000) (the maximum date value in JavaScript).

- const dbAfter = after ? new Date(after) : r.maxval
+ const dbAfter = after ? new Date(after) : new Date(8640000000000000)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const [tooManyInvoices, orgUsers] = await Promise.all([
const [tooManyInvoices, orgUsers] = await Promise.all([
const dbAfter = after ? new Date(after) : new Date(8640000000000000)

Comment on lines 85 to 89
await pg
.with('Org', (qc) => qc.insertInto('Organization').values(org))
.insertInto('OrganizationUser')
.values(orgUsers)
.execute()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure atomicity of database operations.

The PostgreSQL insert operation and the RethinkDB insert operation should be wrapped in a transaction to ensure atomicity and consistency.

-  await pg
-    .with('Org', (qc) => qc.insertInto('Organization').values(org))
-    .insertInto('OrganizationUser')
-    .values(orgUsers)
-    .execute()
+  await pg.transaction().execute(async (trx) => {
+    await trx
+      .with('Org', (qc) => qc.insertInto('Organization').values(org))
+      .insertInto('OrganizationUser')
+      .values(orgUsers)
+      .execute()
+  })
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await pg
.with('Org', (qc) => qc.insertInto('Organization').values(org))
.insertInto('OrganizationUser')
.values(orgUsers)
.execute()
await pg.transaction().execute(async (trx) => {
await trx
.with('Org', (qc) => qc.insertInto('Organization').values(org))
.insertInto('OrganizationUser')
.values(orgUsers)
.execute()
})

Comment on lines 19 to 24
const handleInactive = async (userId: string, dataLoader: DataLoaderWorker) => {
const orgUsers = await dataLoader.get('organizationUsersByUserId').load(userId)
const orgIds = orgUsers.map(({orgId}) => orgId)
await adjustUserCount(userId, orgIds, InvoiceItemType.UNPAUSE_USER, dataLoader).catch(Logger.log)
// TODO: re-identify
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling and address TODO.

The error handling with Logger.log is appropriate. Address the TODO comment to re-identify the user.

Do you want me to address the TODO and implement the re-identify logic, or open a GitHub issue to track this task?

Comment on lines 63 to 68
getKysely()
.updateTable('OrganizationUser')
.set({inactive})
.where('userId', '=', userId)
.where('removedAt', 'is', null)
.execute(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant RethinkDB update operations.

The update operations for OrganizationUser can be refactored to use only PostgreSQL, aligning with the rest of the migration.

- getKysely()
-   .updateTable('OrganizationUser')
-   .set({inactive})
-   .where('userId', '=', userId)
-   .where('removedAt', 'is', null)
-   .execute(),
- r
-   .table('OrganizationUser')
-   .getAll(userId, {index: 'userId'})
-   .filter({removedAt: null})
-   .update({inactive})
-   .run()
+ getKysely()
+   .updateTable('OrganizationUser')
+   .set({inactive})
+   .where('userId', '=', userId)
+   .where('removedAt', 'is', null)
+   .execute()
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
getKysely()
.updateTable('OrganizationUser')
.set({inactive})
.where('userId', '=', userId)
.where('removedAt', 'is', null)
.execute(),
getKysely()
.updateTable('OrganizationUser')
.set({inactive})
.where('userId', '=', userId)
.where('removedAt', 'is', null)
.execute()

Comment on lines 33 to 35
// TEST in Phase 2!
console.log(pgResults)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debug logging.

Remove the debug logging statement before merging the code.

-    console.log(pgResults)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// TEST in Phase 2!
console.log(pgResults)
// TEST in Phase 2!

@@ -11,7 +12,7 @@
{dataLoader}
) => {
const r = await getRethink()

const pg = getKysely()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling for database connection.

Ensure that the database connection is properly handled and closed in case of errors.

+  try {
+    const pg = getKysely()
+  } catch (error) {
+    console.error('Error initializing database connection:', error)
+    throw new Error('Database connection failed')
+  }

Committable suggestion was skipped due to low confidence.

@@ -51,7 +51,7 @@ export async function checkTableEq(
.run()) as RethinkDoc[]
if (rethinkRows.length === 0) break
const lastRow = rethinkRows[rethinkRows.length - 1]!
curUpdatedDate = lastRow.updatedAt
curUpdatedDate = lastRow.joinedAt
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimise the loop condition.

The loop condition for (let i = 0; i < 1e6; i++) is arbitrary. Consider optimising it based on actual data size.

-  for (let i = 0; i < 1e6; i++) {
+  while (true) {

Committable suggestion was skipped due to low confidence.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using a while true loop would open us up to a threat of an infinite loop, right? isn't that worse than an arbitrarily big for loop?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattkrick, you are correct that using a while (true) loop without a proper exit condition can lead to an infinite loop, which is indeed worse than an arbitrarily large for loop. A more robust approach would involve setting a logical exit condition based on the actual data size or another relevant criterion. This way, we can avoid the risk of an infinite loop while also not relying on an arbitrary large number.

Would you like me to propose a specific solution or gather more information on the data size to determine a better exit condition?

Comment on lines 25 to 31
const pgResults = await pg
.selectFrom('OrganizationUser')
.select(({fn}) => ['userId', fn.agg<string[]>('array_agg', ['orgId']).as('orgIds')])
.where('userId', 'in', userIdBatch)
.where('removedAt', 'is', null)
.groupBy('userId')
.execute()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimise query for fetching user IDs.

Consider optimising the query for fetching user IDs to improve performance.

-    const pgResults = await pg
-      .selectFrom('OrganizationUser')
-      .select(({fn}) => ['userId', fn.agg<string[]>('array_agg', ['orgId']).as('orgIds')])
-      .where('userId', 'in', userIdBatch)
-      .where('removedAt', 'is', null)
-      .groupBy('userId')
-      .execute()
+    const pgResults = await pg
+      .selectFrom('OrganizationUser')
+      .select(['userId', pg.raw('array_agg("orgId") AS "orgIds"')])
+      .where('userId', 'in', userIdBatch)
+      .where('removedAt', 'is', null)
+      .groupBy('userId')
+      .execute()
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const pgResults = await pg
.selectFrom('OrganizationUser')
.select(({fn}) => ['userId', fn.agg<string[]>('array_agg', ['orgId']).as('orgIds')])
.where('userId', 'in', userIdBatch)
.where('removedAt', 'is', null)
.groupBy('userId')
.execute()
const pgResults = await pg
.selectFrom('OrganizationUser')
.select(['userId', pg.raw('array_agg("orgId") AS "orgIds"')])
.where('userId', 'in', userIdBatch)
.where('removedAt', 'is', null)
.groupBy('userId')
.execute()

Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
@github-actions github-actions bot added size/xl and removed size/l labels Jul 10, 2024
Copy link
Contributor

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
packages/server/graphql/public/mutations/setOrgUserRole.ts (1)

98-98: Ensure consistency in role updates between PostgreSQL and RethinkDB.

The role updates in the setOrgUserRole function are performed sequentially without ensuring atomicity across the two databases. If one update fails while the other succeeds, it can lead to inconsistencies.

  • The function updates the role in PostgreSQL first and then in RethinkDB.
  • There is no rollback mechanism to handle failures in one database after a successful update in the other.

Consider implementing a transaction management strategy or a rollback mechanism to ensure both updates succeed or fail together.

Analysis chain

Ensure consistency in role updates between PostgreSQL and RethinkDB.

The role update in PostgreSQL should be consistent with the role update in RethinkDB. Verify that both updates succeed or fail together.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistency in role updates between PostgreSQL and RethinkDB.

# Test: Ensure the role updates succeed or fail together.
rg --type ts 'setOrgUserRole'

Length of output: 1044


Script:

#!/bin/bash
# Searching for the implementation of the setOrgUserRole function to verify consistency in role updates between PostgreSQL and RethinkDB.
ast-grep --lang typescript --pattern $'const setOrgUserRole: MutationResolvers[\'setOrgUserRole\'] = async ($_$) => { $$$ }' packages/server/graphql/public/mutations/setOrgUserRole.ts

Length of output: 188


Script:

#!/bin/bash
# Searching for the implementation of the setOrgUserRole function and including context lines to inspect the implementation details.
rg 'const setOrgUserRole' -A 50 packages/server/graphql/public/mutations/setOrgUserRole.ts

Length of output: 1928


Script:

#!/bin/bash
# Extending the context lines to capture the complete implementation of the setOrgUserRole function.
rg 'const setOrgUserRole' -A 100 packages/server/graphql/public/mutations/setOrgUserRole.ts

Length of output: 3226

@github-actions github-actions bot added size/m and removed size/xl labels Jul 11, 2024
@mattkrick mattkrick merged commit 89d4c4f into master Jul 11, 2024
6 checks passed
@mattkrick mattkrick deleted the chore/organizationUser-phase2 branch July 11, 2024 17:01
@github-actions github-actions bot mentioned this pull request Jul 11, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant